Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

feat: add (x | 1) optimization for booleans #6795

Merged
merged 8 commits into from
Dec 12, 2024
Merged

feat: add (x | 1) optimization for booleans #6795

merged 8 commits into from
Dec 12, 2024
+16 −11

Conversation

jfecher
Copy link
Contributor

@jfecher jfecher commented Dec 12, 2024

Description

Problem*

Resolves an issue in a slack thread by @michaeljklein

Summary*

Breakout of a change in #6771 where slice pop back seemed to be using all of its ctrl_typevars for a tuple instead of the one element that was actually being popped. This only affects the ArrayGet though which is subsequently optimized out.

Additional Context

Documentation*

Check one:

  • No documentation needed.
  • Documentation included in this PR.
  • [For Experimental Features] Documentation to be submitted in a separate PR.

PR Checklist*

  • I have tested the changes locally.
  • I have formatted the changes with Prettier and/or cargo fmt on default settings.

@jfecher jfecher requested a review from a team December 12, 2024 17:38
@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Dec 12, 2024 •
edited
Loading

Changes to number of Brillig opcodes executed

Generated at commit: 9f0fab59e72cb848619959527679ebf75db780c1, compared to commit: 919149d3413be5232b33611094687fdb5fd86086

🧾 Summary (10% most significant diffs)

Program Brillig opcodes (+/-) %
bool_or -2 ✅ -8.00%
Full diff report 👇
Program Brillig opcodes (+/-) %
bool_or 23 (-2) -8.00%

@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Dec 12, 2024 •
edited
Loading

Changes to Brillig bytecode sizes

Generated at commit: 9f0fab59e72cb848619959527679ebf75db780c1, compared to commit: 919149d3413be5232b33611094687fdb5fd86086

🧾 Summary (10% most significant diffs)

Program Brillig opcodes (+/-) %
bool_or -4 ✅ -12.90%
Full diff report 👇
Program Brillig opcodes (+/-) %
bool_or 27 (-4) -12.90%

@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Dec 12, 2024 •
edited
Loading

Peak Memory Sample

Program Peak Memory
keccak256 79.14M
workspace 121.92M
regression_4709 295.98M
ram_blowup_regression 2.44G
private-kernel-tail 210.43M
private-kernel-reset 862.21M
private-kernel-inner 307.94M
parity-root 175.78M

@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Dec 12, 2024 •
edited
Loading

Compilation Sample

Program Compilation Time %
sha256_regression 0m1.528s 5%
regression_4709 0m0.832s 9%
ram_blowup_regression 0m17.393s 1%
private-kernel-tail 0m1.154s -7%
private-kernel-reset 0m9.373s 3%
private-kernel-inner 0m2.803s 3%
parity-root 0m0.911s -4%
noir-contracts 2m58.779s 9%

michaeljklein
michaeljklein approved these changes Dec 12, 2024
View reviewed changes
Copy link
Contributor

@michaeljklein michaeljklein left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@jfecher
Copy link
Contributor Author

jfecher commented Dec 12, 2024 •
edited
Loading 

$ cargo t partial_constant_folding
   Compiling noirc_evaluator v1.0.0-beta.0 (/home/jake/Code/Noir/noir/compiler/noirc_evaluator)
    Finished test [optimized + debuginfo] target(s) in 1.09s
     Running unittests src/lib.rs (/home/jake/Code/Noir/noir/target/debug/deps/noirc_evaluator-4693fae089867dbc)

running 0 tests

test result: ok. 0 passed; 0 failed; 0 ignored; 0 measured; 189 filtered out; finished in 0.00s

I must be missing something obvious. Not sure why partial_constant_folding is failing in CI but not being run at all locally for me.

Edit: Maybe it is the #[cfg(feature = "bn254")]

@jfecher jfecher changed the title fix: Fix possible type error & add (x | 1) optimization for booleans feat: add (x | 1) optimization for booleans Dec 12, 2024
@jfecher jfecher enabled auto-merge December 12, 2024 19:45
@jfecher jfecher added this pull request to the merge queue Dec 12, 2024
Merged via the queue into master with commit feec2a1 Dec 12, 2024
64 checks passed
@jfecher jfecher deleted the jf/fix-simplify-types branch December 12, 2024 21:22
@noirwhal noirwhal mentioned this pull request Dec 12, 2024
Open
AztecBot added a commit to AztecProtocol/aztec-packages that referenced this pull request Dec 13, 2024
@AztecBot
[1 changes] feat(ssa): Hoist MakeArray instructions during loop invar…
2fe87c2 
…iant code motion (noir-lang/noir#6782)

feat: add `(x | 1)` optimization for booleans (noir-lang/noir#6795)
feat: `nargo test -q` (or `nargo test --format terse`) (noir-lang/noir#6776)
fix: disable failure persistance in nargo test fuzzing (noir-lang/noir#6777)
feat(cli): Verify `return` against ABI and `Prover.toml` (noir-lang/noir#6765)
chore(ssa): Activate loop invariant code motion on ACIR functions (noir-lang/noir#6785)
fix: use extension in docs link so it also works on GitHub (noir-lang/noir#6787)
fix: optimizer to keep track of changing opcode locations (noir-lang/noir#6781)
fix: Minimal change to avoid reverting entire PR #6685 (noir-lang/noir#6778)
AztecBot added a commit to AztecProtocol/aztec-packages that referenced this pull request Dec 13, 2024
@AztecBot
feat(ssa): Hoist MakeArray instructions during loop invariant code mo…
89e4859 
…tion (noir-lang/noir#6782)

feat: add `(x | 1)` optimization for booleans (noir-lang/noir#6795)
feat: `nargo test -q` (or `nargo test --format terse`) (noir-lang/noir#6776)
fix: disable failure persistance in nargo test fuzzing (noir-lang/noir#6777)
feat(cli): Verify `return` against ABI and `Prover.toml` (noir-lang/noir#6765)
chore(ssa): Activate loop invariant code motion on ACIR functions (noir-lang/noir#6785)
fix: use extension in docs link so it also works on GitHub (noir-lang/noir#6787)
fix: optimizer to keep track of changing opcode locations (noir-lang/noir#6781)
fix: Minimal change to avoid reverting entire PR #6685 (noir-lang/noir#6778)
This was referenced Dec 13, 2024
Merged
Open
@TomAFrench TomAFrench mentioned this pull request Dec 13, 2024
Open
TomAFrench added a commit that referenced this pull request Dec 14, 2024
@TomAFrench
Merge branch 'master' into ab/optimize-array-get-for-if-else-value
33ba5cf 
* master: (313 commits)
  chore: Do not print entire functions when running debug trace (#6814)
  chore(ci): Active rollup circuits in compilation report (#6813)
  feat(ssa): Bring back tracking of RC instructions during DIE (#6783)
  feat: add `nargo test --format json` (#6796)
  chore: Change Id to use a u32 (#6807)
  feat(ssa): Hoist MakeArray instructions during loop invariant code motion  (#6782)
  feat: add `(x | 1)` optimization for booleans (#6795)
  feat: `nargo test -q` (or `nargo test --format terse`) (#6776)
  fix: disable failure persistance in nargo test fuzzing (#6777)
  feat(cli): Verify `return` against ABI and `Prover.toml` (#6765)
  chore(ssa): Activate loop invariant code motion on ACIR functions (#6785)
  fix: use extension in docs link so it also works on GitHub (#6787)
  fix: optimizer to keep track of changing opcode locations (#6781)
  fix: Minimal change to avoid reverting entire PR #6685 (#6778)
  feat: several `nargo test` improvements (#6728)
  chore: Try replace callstack with a linked list (#6747)
  chore: Use `NumericType` not `Type` for casts and numeric constants (#6769)
  chore(ci): Extend compiler memory report to external repos (#6768)
  chore(ci): Handle external libraries in compilation timing report (#6750)
  feat(ssa): Implement missing brillig constraints SSA check (#6658)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@michaeljklein michaeljklein michaeljklein approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@jfecher @michaeljklein